Conversation
- YouTubeProvider to handle context. - Accepting search parameter to useVideos.
- VideoDetail displaying title and description. - useRelatedVideos hook. - layout improved. - RelatedVideos in progres...
- handling error from YouTube API
PacoMojica
left a comment
PacoMojica
left a comment
There was a problem hiding this comment.
Hello Alfredo, I like what you have up to this point. One thing that is missing is some tests. I see a lot of 'should render{name} component correctly' but you can test some events, the custom hooks, or the providers, I'd like improvements in the next delivery. I left some comments, just minor things. Great work 👍
| import React from 'react'; | ||
| import { ThemeProvider } from 'styled-components'; | ||
| import { useYouTube } from '../YouTube/YouTubeProvider'; | ||
|
|
||
| const theme = { | ||
| dark: { | ||
| primary: '#4396f3', | ||
| secondary: '#4a4646', | ||
| color: '#f7f7f7', | ||
| }, | ||
| }; | ||
|
|
||
| function MyThemeProvider({ children }) { | ||
| const { state } = useYouTube(); | ||
| const { theme: stateTheme } = state; | ||
| return <ThemeProvider theme={theme[stateTheme] || {}}>{children}</ThemeProvider>; | ||
| } | ||
|
|
||
| export default MyThemeProvider; |
| const url = ['https://www.googleapis.com/youtube/v3/search?']; | ||
| url.push(`key=${process.env.REACT_APP_YOUTUBE_API_KEY}`); | ||
| url.push(`&part=snippet&maxResults=10&type=video`); | ||
| if (search) url.push(`&q=${search}`); |
There was a problem hiding this comment.
Took me a while to catch why this was an array, I think that concatenating strings by using the + operator is more expressive, at first sight you know that 'asdf' + 'ewer' = 'asdfqwer' and in this case you have an array of strings, you push items and at the end you do url.join('') which I think is somewhat convoluted, at the end of the day it is just preference
| const [videos, setVideos] = useState([]); | ||
| const [isLoading, setLoading] = useState(false); | ||
| const [error, setError] = useState(null); |
There was a problem hiding this comment.
Great, I like that you are handling the data, the loading state and the possible errors
| return { videos, isLoading, error }; | ||
| } | ||
|
|
||
| export default useRelatedVideos; |
There was a problem hiding this comment.
Both of the hooks, the useReleatedVideos and useVideos, are well defined 👍
| case 'search': | ||
| return { | ||
| ...state, | ||
| search: action.payload, | ||
| }; | ||
| case 'currentVideo': | ||
| return { | ||
| ...state, | ||
| currentVideo: action.payload, | ||
| }; | ||
| case 'closeCurrentVideo': | ||
| return { | ||
| ...state, | ||
| currentVideo: null, | ||
| }; | ||
| case 'switchTheme': | ||
| return { | ||
| ...state, | ||
| theme: state.theme === 'dark' ? 'default' : 'dark', | ||
| }; |
| theme: state.theme === 'dark' ? 'default' : 'dark', | ||
| }; | ||
| default: { | ||
| return state; |
There was a problem hiding this comment.
I'd throw an error here, it can prevent some unwanted stuff to go unnoticed, for example you can pass CloseCurrentVideo and the state won't update but it will be hard to find that the bug is caused because the C should be lower case like closeCurrentVideo, I learned it the hard way 😆
2 participants
Hi,
This PR contains both mini-challenges: 3 and 4.
Thanks a lot.